Add opt-in rules to detect plugin managers without attribute discovery by bbrala · Pull Request #1068 · mglaman/phpstan-drupal

bbrala · 2026-11-05T17:12:17Z

Hi! AI was used to make this, i've tried to verify against contrib best i could to make sure we don't screw ourselves over. The results were quite promising. This should help up to 500 projects in contrib with deprecation status and project analysis.

Triggered by https://drupal.slack.com/archives/C08QW79LJ94/p1793842732050899 which was already mentioned to me by berdir in https://drupal.slack.com/archives/C08QW79LJ94/p1787647148732209.

Introduction

Core now throws on plugin managers that only support annotation discovery (#3577900, CR 3522776). Nothing in the PHPStan or Upgrade Status toolchain sees it: the check in DefaultPluginManager::__construct() is a runtime @trigger_error, not a @deprecated tag. The result is bot runs that call a module "Drupal 12 compatible" when its managers will fatal. json_ld_schema is one example.

This PR adds the missing static check.

What it does

It reports a DefaultPluginManager subclass whose parent::__construct() receives a non-attribute class as the 5th argument ($plugin_definition_attribute_name). The 5th argument can be positional or the named argument.

There are two rules behind a single toggle:

PluginManagerAttributeDiscoveryRule handles a literal ::class, a string literal, or a local variable that holds one. Identifier: pluginManager.attributeDiscoveryMissing.
PluginManagerAttributeDiscoveryDefaultRule handles a static::$prop / self::$prop default, or a forwarded constructor parameter default. Identifier: pluginManager.attributeDiscoveryMissingDefault. Its message says the default may be overridden by a subclass or caller, because that can't be seen statically.

Both are off by default. Enable them with:

parameters:
    drupal:
        rules:
            pluginManagerAttributeDiscoveryRule: true

Both rules do nothing when AttributeInterface can't be resolved (Drupal older than 10.2). A manager that omits the 5th argument is not reported, matching core. The fix is to pass an Attribute class as the 5th argument and keep the Annotation class as the 6th. That works from 10.2.0.

Why opt-in

Most existing rules are now on by default, so I'd normally follow that. I kept this one off because it can't be fixed on a module that still supports 10.0 or 10.1, and I'd rather you make that call. Given the numbers below I'm happy to flip the default if you want it on.

Validation

I ran it against the 100 highest-install contrib projects that a code-index query flagged as having annotation-only managers (dev branches, Drupal 11 core).

122 hits across 88 projects.
Every hit was checked by reading the constructor: 121 true positives, 1 questionable, 0 false positives.
The questionable hit is bootstrap_layouts. The module is affected, but the flagged default sits on an abstract base class and both concrete managers pass an explicit annotation.
Of the 90 projects that are really affected, the rules found 88. Misses: bootstrap (its manager never calls parent::__construct()) and graphql (annotation class supplied only in services.yml).
10 of the 100 projects the query flagged were not affected (YamlDiscovery managers, or already on attributes). The rules stayed silent on those.

Known limitations

Managers defined only in services.yml (class: DefaultPluginManager with all arguments in YAML) aren't detected, and neither is new DefaultPluginManager(...). ServiceMap drops arguments:, so covering this needs a change there. I'd do that separately.
The default rule can't see subclasses or callers that override the default, or writes to a property or parameter outside the analysed class.

Tests

The new rule tests pass (4 of 4). The full suite has one failure: InspectorTypeExtensionTest bug-857. It also fails on a clean main and is not touched by this branch.

…ameter defaults

… is unavailable

mglaman

Thanks for this, and for the validation run against contrib. 121 true positives and no false positives is a strong result for a new rule. I checked the behavior against core: on 11.4 a non-attribute 5th argument triggers the deprecation, on main it throws InvalidArgumentException, and an omitted or null argument does neither, so skipping that case is right. The tests pass merged onto current main.

Let's keep it opt-in. Since you run the project update bot, turning it on there gets the coverage this is for.

One fix and a few smaller things:

The inline comment below: the default rule can pick up the wrong constructor.
nit: make the two concrete rules final.
nit: add pluginManagerAttributeDiscoveryRule to the README's drupal.rules docs so people can find it.
Not blocking: for the forwarded-parameter case, ParameterReflection::getDefaultValue() at the call site might replace some of the manual AST walking. Fine to leave as is given the results.

mglaman · 2026-11-05T19:13:09Z

+
+        $classNode = $node->getOriginalNode();
+        $finder = new NodeFinder();
+        $constructor = $finder->findFirst(


findFirst() descends into method bodies, so an anonymous class with its own __construct in a method above the real constructor gets picked instead. ClassLike::getMethod() only looks at the class's own methods:

$constructor = $classNode->getMethod('__construct'); if ($constructor === null) { return []; }

…sted scopes

bbrala · 2026-11-05T20:10:52Z

Reviewed the comments, confirmed and fixed :)

goba · 2026-11-06T15:52:14Z

Here is my LLM agents's review as it was about to write a regex custom checker for this and I thought it should review this PR instead :D It affects 9 projects with 15 instances of the error in Drupal CMS, so this will be likely useful for contrib.

I tried this on the projects from our Drupal 12 readiness work for Drupal CMS. It works well, and I would like to use it in Upgrade Status.

Setup: this PR at 39013bf, PHPStan 2.3.5, Drupal 11.5.x-dev, level 0, with pluginManagerAttributeDiscoveryRule: true. I ran it on 75 unpatched contrib modules and themes.

Result: we had fixed 9 of these projects by hand for this problem: crop, ctools, field_group, field_validation, google_tag, key, scheduler, symfony_mailer_lite and webform. The rule found all 9 and no other project, with 15 errors in total. There were no false positives.

Some of these are hard to find without type inference. A regex prototype of mine only found 7 of the 9.

key: $type_annotations[$type] gives one correct error per array value.
scheduler: the class comes through a local variable.
google_tag: it has a ::class that looks like an attribute, but the import is from the Annotation namespace.
canvas: AdapterManager passes the attribute class and then the annotation string. It is correctly not reported.

All 15 errors came from PluginManagerAttributeDiscoveryRule. The default value rule had no hits on these projects, so only the small test below exercised it.

Edge cases. I also ran a small test module with these managers:

Pattern	Reported	Correct for Drupal 12?
Annotation as string, `::class`, named argument or `self::CONST`	Yes	Yes
Annotation through an abstract parent without a constructor	Yes	Yes
Annotation in a constructor from a trait	Yes	Yes
Constructor parameter default or static property default	Yes	Yes
Attribute only, or attribute and annotation	No	Yes
Fifth argument omitted	No	Yes, core does not throw
Child of a manager that is already fixed	No	Yes
`static::CONST`	No	Missed, core throws
`$this->property`, a method call or `...$args`	No	Missed, core throws
A string that names a class that doesn't exist	No	Missed, core throws
`class_exists(Attr::class) ? Attr::class : Annotation::class`	Yes	False positive, works on 12
`version_compare()` branch with the annotation in the old branch	Yes	False positive, works on 12

I don't think any of these should block this PR:

static::CONST could fit in the default value rule, as it handles static::$property the same way. None of the Drupal CMS projects use this.
The other misses fit the rule's approach of only reporting when certain.
None of our 75 Drupal CMS projects use the two false positive patterns. They can be ignored by identifier, so a note in the README may be enough.

Upgrade Status integration. I tried this on a local branch, and it needed three small changes:

The parameter can only be set when the rule exists. On phpstan-drupal 2.2.2 it fails with Unexpected item 'parameters › drupal › rules › pluginManagerAttributeDiscoveryRule'. So Upgrade Status should check class_exists() on the rule class before it adds the parameter. This works as it is, so nothing needs to change here.
Upgrade Status only looked for a capitalized Deprecated in drupal:X to categorize messages. This rule uses core's is deprecated in drupal:X wording. I think that wording is right, so Upgrade Status needs to be changed to accept it.
Upgrade Status drops PHPStan tips, and the tip here says how to fix the problem. I added it to the message based on the pluginManager.attributeDiscovery identifier prefix. So it would help if these identifiers stay stable.

With those changes, a test module with one annotation manager and one attribute manager reports only the annotation manager. This passes on Drupal 11.5 and on 12.0.

Opt-in works fine for us. Once this is in a release, Upgrade Status can turn the rule on, and its users will get this check for Drupal 12.

bbrala added 3 commits November 5, 2026 18:15

Add opt-in rule to detect plugin managers without attribute discovery

1382c2a

Detect plugin manager attribute argument from static property and par…

52abd10

…ameter defaults

Skip plugin manager attribute discovery rules when AttributeInterface…

8b5f49b

… is unavailable

bbrala force-pushed the feature/plugin-manager-attribute-discovery branch from aff9260 to 8b5f49b Compare November 5, 2026 17:16

mglaman reviewed Nov 5, 2026

View reviewed changes

bbrala added 4 commits November 5, 2026 21:03

Find the plugin manager constructor and its calls without entering ne…

ae26b95

…sted scopes

Make the plugin manager attribute discovery rules final

2a1ef6c

Document the pluginManagerAttributeDiscoveryRule parameter

b78722b

Merge branch 'main' into feature/plugin-manager-attribute-discovery

39013bf

Navigation Menu

Repository navigation

Add opt-in rules to detect plugin managers without attribute discovery - #1068

Add opt-in rules to detect plugin managers without attribute discovery#1068
bbrala wants to merge 7 commits into
mglaman:mainfrom
bbrala:feature/plugin-manager-attribute-discovery

bbrala commented Nov 5, 2026 •

edited

Loading

Uh oh!

mglaman left a comment

Uh oh!

mglaman Nov 5, 2026

Uh oh!

bbrala commented Nov 5, 2026

Uh oh!

goba commented Nov 6, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

bbrala commented Nov 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Introduction

What it does

Why opt-in

Validation

Known limitations

Tests

Uh oh!

mglaman left a comment

Choose a reason for hiding this comment

Uh oh!

mglaman Nov 5, 2026

Choose a reason for hiding this comment

Uh oh!

bbrala commented Nov 5, 2026

Uh oh!

goba commented Nov 6, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bbrala commented Nov 5, 2026 •

edited

Loading